Move constants required by UserInterface into interface definition #8896

klada · 2018-03-20T10:16:40Z

The constants which need to be checked in implementsActions formerly
resided in the private \OC\User\Backend class. Since they need to be
accessed in apps they should actually reside in the public interface.

Closes #8895

codecov · 2018-03-20T10:47:29Z

Codecov Report

Merging #8896 into master will decrease coverage by 20.32%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master    #8896       +/-   ##
=============================================
- Coverage     51.82%   31.49%   -20.33%     
  Complexity    25281    25281               
=============================================
  Files          1604     1604               
  Lines         94804    94804               
  Branches       1377     1377               
=============================================
- Hits          49131    29863    -19268     
- Misses        45673    64941    +19268

Impacted Files	Coverage Δ	Complexity Δ
lib/private/User/Backend.php	`61.53% <ø> (ø)`	`12 <0> (ø)`	⬇️
apps/dav/lib/Connector/Sabre/Server.php	`0% <0%> (-100%)`	`1% <0%> (ø)`
lib/private/Files/Mount/CacheMountProvider.php	`0% <0%> (-100%)`	`4% <0%> (ø)`
apps/user_ldap/lib/Settings/Admin.php	`0% <0%> (-100%)`	`5% <0%> (ø)`
apps/user_ldap/lib/BackendUtility.php	`0% <0%> (-100%)`	`1% <0%> (ø)`
apps/files_trashbin/lib/AppInfo/Application.php	`0% <0%> (-100%)`	`2% <0%> (ø)`
lib/public/Comments/CommentsEvent.php	`0% <0%> (-100%)`	`3% <0%> (ø)`
lib/private/Route/CachingRouter.php	`0% <0%> (-100%)`	`3% <0%> (ø)`
...rivate/Authentication/Token/DefaultTokenMapper.php	`0% <0%> (-100%)`	`11% <0%> (ø)`
apps/user_ldap/lib/Migration/UUIDFixUser.php	`0% <0%> (-100%)`	`1% <0%> (ø)`
... and 370 more

MorrisJobke · 2018-03-20T18:14:52Z

lib/public/UserInterface.php

@@ -39,14 +39,25 @@
 * @since 4.5.0
 */
 interface UserInterface {
+	/**
+	 * actions that user backends can define


Please add a @since 14.0.0 tag so everybody knows when this was added and with what version it could be used.

MorrisJobke · 2018-03-20T18:19:00Z

I still don't like the overall approach of how we implemented the user backends. I have in mind, that we discussed a bit more about distinct user backend interfaces for all of those constants. Something like SetUserNameInterface and then the interfaces defines that there is a setUserName method and you implement one or many of those interfaces. In this way we properly utilize language features instead of reimplementing what the language construct "interface" already provides.

As short term solution this still is okay, but we maybe should think about how to make it properly maintainable. ;)

rullzer · 2018-03-20T18:26:23Z

I agree with Morris that we should move away from this. And then adding it to the public interface doesn't make it easier as we have a long depreciation period.

klada · 2018-03-21T07:34:16Z

As I pointed out in #8895 this is just a suggestion. Of course I am not in the position to decide which interface is supposed to be public or not, but let me explain my motivation:

I have written my own user backend which implements these public interfaces. Whenever I look at the code I feel quite good about the fact that I am only using public interfaces. There is only one place where the UserBackend interface forces me to access the private Nextcloud API (when I need to check against these constants). This always makes me think that my code can break any time soon and right now there is no way around that. And this also applies to user_ldap and user_saml.

And I also agree that the current situation with the user backend stuff is far from optimal at the moment. That's why I also suggested in #8895 that another approach could be to provide a public abstract user backend class instead of just the interface definitions. More code from \OC\User\Backend could then be reused by existing backends (such as implementsActions which would remove the need to add these constants to the public interface). But that would certainly require more code reorganization.

nickvergessen · 2018-03-21T08:23:09Z

lib/public/UserInterface.php


 	/**
 	 * Check if backend implements actions
 	 * @param int $actions bitwise-or'ed actions
 	 * @return boolean
 	 *
 	 * Returns the supported actions as int to be
-	 * compared with \OC\User\Backend::CREATE_USER etc.
+	 * compared with CREATE_USER etc.


self::CREATE_USER but doesn't matter a lot I guess

@nickvergessen You are right, I have updated the comment and rebased my PR in order to keep the number of commits down.

The constants which need to be checked in `implementsActions` formerly resided in the private \OC\User\Backend class. Since they need to be accessed in apps they should actually reside in the public interface. Signed-off-by: Daniel Klaffenbach <daniel.klaffenbach@hrz.tu-chemnitz.de>

MorrisJobke · 2018-03-21T09:11:59Z

Let's wait for CI.

MorrisJobke · 2018-03-21T09:17:57Z

Had a chat with @rullzer on IRC and it would maybe make sense to move the dedicated interfaces approach forward. Because this then would be a clean approach to tackle the underlying problem.

MorrisJobke · 2018-03-21T09:24:37Z

@klada Let's leave this open for some days and we will look into how easy it is to go the proper way.

MorrisJobke

Blocking. See my previous comment.

MorrisJobke

Blocking 😉

rullzer · 2018-03-22T08:21:15Z

See #8935

rullzer · 2018-03-22T09:55:51Z

Ok lets close this then and continue in #8935

MorrisJobke reviewed Mar 20, 2018

View reviewed changes

MorrisJobke requested review from rullzer, nickvergessen and blizzz March 20, 2018 18:15

MorrisJobke added 3. to review Waiting for reviews feature: users and groups enhancement labels Mar 20, 2018

MorrisJobke added this to the Nextcloud 14 milestone Mar 20, 2018

klada force-pushed the interface_fixes branch from 655ec61 to 9ed1380 Compare March 21, 2018 08:09

nickvergessen reviewed Mar 21, 2018

View reviewed changes

nickvergessen approved these changes Mar 21, 2018

View reviewed changes

klada force-pushed the interface_fixes branch from 9ed1380 to f7aa102 Compare March 21, 2018 08:38

MorrisJobke approved these changes Mar 21, 2018

View reviewed changes

MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 21, 2018

MorrisJobke approved these changes Mar 21, 2018

View reviewed changes

MorrisJobke requested changes Mar 21, 2018

View reviewed changes

klada mentioned this pull request Mar 22, 2018

Backend interfaces #8935

Merged

rullzer closed this Mar 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move constants required by UserInterface into interface definition #8896

Move constants required by UserInterface into interface definition #8896

klada commented Mar 20, 2018

codecov bot commented Mar 20, 2018 •

edited

Loading

MorrisJobke Mar 20, 2018

MorrisJobke commented Mar 20, 2018

rullzer commented Mar 20, 2018

klada commented Mar 21, 2018

nickvergessen Mar 21, 2018

klada Mar 21, 2018

MorrisJobke commented Mar 21, 2018

MorrisJobke commented Mar 21, 2018

MorrisJobke commented Mar 21, 2018

MorrisJobke left a comment

MorrisJobke left a comment

rullzer commented Mar 22, 2018

rullzer commented Mar 22, 2018

Move constants required by UserInterface into interface definition #8896

Move constants required by UserInterface into interface definition #8896

Conversation

klada commented Mar 20, 2018

codecov bot commented Mar 20, 2018 • edited Loading

Codecov Report

MorrisJobke Mar 20, 2018

Choose a reason for hiding this comment

MorrisJobke commented Mar 20, 2018

rullzer commented Mar 20, 2018

klada commented Mar 21, 2018

nickvergessen Mar 21, 2018

Choose a reason for hiding this comment

klada Mar 21, 2018

Choose a reason for hiding this comment

MorrisJobke commented Mar 21, 2018

MorrisJobke commented Mar 21, 2018

MorrisJobke commented Mar 21, 2018

MorrisJobke left a comment

Choose a reason for hiding this comment

MorrisJobke left a comment

Choose a reason for hiding this comment

rullzer commented Mar 22, 2018

rullzer commented Mar 22, 2018

codecov bot commented Mar 20, 2018 •

edited

Loading